-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add lock icon for disabled and readonly states #189
Conversation
🦋 Changeset detectedLatest commit: 4e80e9e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Preview URLsEnv: preview |
@@ -204,7 +204,7 @@ Target the error block via `data-error`. | |||
<div class='mb-4 w-64'> | |||
<Form::Fields::Checkbox | |||
> | |||
<:label>Label <svg class="inline" xmlns="http://www.w3.org/2000/svg" width="24" height="24" stroke="currentColor" viewBox="0 0 24 24"><path d="M12 3a9 9 0 11-6.364 2.636A8.972 8.972 0 0112 3zm0 4.7v5.2m0 3.39v.01" fill="none" stroke-linecap="round" stroke-linejoin="round" stroke-width="2"></path></svg></:label> | |||
<:label>Label <svg class="inline w-4 h-4 -mt-1" xmlns="http://www.w3.org/2000/svg" width="24" height="24" stroke="currentColor" viewBox="0 0 24 24"><path d="M12 3a9 9 0 11-6.364 2.636A8.972 8.972 0 0112 3zm0 4.7v5.2m0 3.39v.01" fill="none" stroke-linecap="round" stroke-linejoin="round" stroke-width="2"></path></svg></:label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
390af0f
to
f9a52a6
Compare
packages/ember-toucan-core/src/components/form/fields/checkbox-group.hbs
Outdated
Show resolved
Hide resolved
export interface ToucanLockIconComponentSignature { | ||
Element: SVGElement; | ||
Args: {}; | ||
Blocks: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No way to get the type system to fail to check if the consumer passes a block, eh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be missing something somewhere, but I tried Blocks: never
and glint
still passed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Womp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it has to be Blocks: { default: never }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! you were right! TIL! Thanks a bunch, @nicolechung !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting this with Blocks: { default: never }
:
Type 'never' must have a '[Symbol.iterator]()' method that returns an iterator.
The error seems to be coming from our use of never
rather than the fact that the consumer is passing a default block. I think the error we'd want to see would read something like "Argument of type [...] is not assignable to parameter of type 'never'".
What's less bad? Not producing a typechecking error when we should be or producing a wrong and possibly confusing error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked over Zoom - we reverted!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the RFC it says if you have no blocks then it might work, but I'm not sure if this has been implemented.
https://rfcs.emberjs.com/id/0748-glimmer-component-signature/#blocks
packages/ember-toucan-core/src/components/form/fields/checkbox.ts
Outdated
Show resolved
Hide resolved
packages/ember-toucan-core/src/components/form/fields/checkbox-group.hbs
Outdated
Show resolved
Hide resolved
c12955e
to
0a70f14
Compare
This reverts commit bef5d3d.
🚀 Description
This PR adds the lock icon for disabled and readonly states according to our designs.
🔬 How to Test
📸 Images/Videos of Functionality
Iconography
While I was in here, I adjusted some of the iconography in our demos to be a bit closer to what we'd prefer.
Readonly + Disabled Locks
A few before and after pictures of the readonly and disabled states.